chore: add gitguardian config to exclude compliance test key fixtures#33
chore: add gitguardian config to exclude compliance test key fixtures#33MichielDean wants to merge 2 commits into
Conversation
The AdCP compliance test vectors in adcp-server/src/test/resources/compliance/ contain intentionally public test keys with _private_d_for_test_only fields. These are for signer/verifier round-trip conformance testing and MUST NOT be used in production. The keys.json files carry explicit warnings in their _WARNING and fields. GitGuardian correctly detects these as private key material, but they are false positives — the keys are published in the AdCP spec repo at https://adcontextprotocol.org/compliance/latest/test-vectors/ and are intentionally public for cross-SDK conformance testing.
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
I think this needs another pass before approval.
Findings:
-
.gitguardiandoes not configure the GitGuardian GitHub check run this PR is trying to quiet. GitGuardian's local ggshield config is documented as.gitguardian.yamlwithversion: 2andsecret.ignored_paths; the top-levelpaths-ignore/matches-ignoreshape here is legacy ggshield config, and ggshield ignores are not shared with the GitGuardian dashboard/check-run integration. If the goal is to suppress the GitGuardian Security Checks status, this should be configured via the GitGuardian dashboard filepath exclusion rules, or the repo should add the actual ggshield config only if a local/CI ggshield invocation consumes it. -
The ignore patterns point at
adcp-server/src/test/resources/compliance/..., but that path does not exist on eitherorigin/mainor this PR head. This makes the rule a no-op for the current repository state, even before considering whether GitGuardian will read the file. Please align the exclusion with the actual fixture path when those files are added, or include this config in the same PR that adds the fixtures so the pattern can be reviewed against real files.
One smaller security concern once the path issue is fixed: **/*hmac* is broad for a secret-scan bypass. Prefer excluding exact public test-vector files rather than every future compliance file whose name contains hmac.
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
…erns Replace legacy v1 .gitguardian.yml (paths-ignore/matches-ignore) with documented v2 .gitguardian.yaml (version: 2 + secret.ignored_paths), which is what ggshield actually loads (docs.gitguardian.com/ggshield-docs/configuration). Drop the **/*hmac* pattern: no fixture is named *hmac* today, so the rule is a no-op; keeping it would silently exclude unrelated future files whose names happen to contain 'hmac'. Scope ignores to the exact public test-vector key files instead of the compliance/** glob. Note in the file that this configures ggshield (local/CI CLI scans), not the 'GitGuardian Security Checks' GitHub App check-run, which is driven by dashboard filepath exclusion rules and is unaffected by any repo-local config. Supersedes PR #33, which added a duplicate .gitguardian JSON file with the same v1 patterns.
|
Closing as duplicate of #32, which now carries a corrected ggshield config. Review findings on this PR (all confirmed against repo state):
The fix landed in #32 (commit adcp-sdk-java@4d09593):
To actually turn the failing GitGuardian Security Checks green on #32, a workspace admin needs to add filepath exclusions in the GitGuardian dashboard (Secrets Detection → Exclusion rules → Filepath) for the two |
2 participants
Summary
Add
.gitguardianconfig to suppress false-positive secret detections on the AdCP compliance test vectors.The test vectors in
adcp-server/src/test/resources/compliance/contain intentionally public test keys with_private_d_for_test_onlyfields. These ship in the official AdCP spec repo athttps://adcontextprotocol.org/compliance/latest/test-vectors/and are designed for cross-SDK conformance testing. Thekeys.jsonfiles carry explicit_WARNINGand$commentbanners.GitGuardian correctly detects these as private key material, but they are false positives — the keys are deliberately public and must not be treated as secrets.
This PR adds a
.gitguardianconfig withpaths-ignorefor the compliance test fixtures so that PR #32 (Track 4 L1 signing) and future PRs that reference these vectors don't trigger false-positive security alerts.Note: This needs to be on
mainbefore the.gitguardianpaths-ignore takes effect for PR checks.